Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wf: handle "livelock" checking before reaching WfPredicates::compute. #70170

Merged
merged 2 commits into from
May 2, 2020
Merged

wf: handle "livelock" checking before reaching WfPredicates::compute. #70170

merged 2 commits into from
May 2, 2020

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Mar 19, 2020

For wf::obligations's "livelock" handling, this PR shouldn't cause any behavioral changes, as the check moved to it should be equivalent to the old one in WfPredicates::compute.

However, it fixes #70168 by making other users of WfPredicates::compute (that is, wf::predicate_obligations and compute's own upvar handling) correct for ty::Infer, in that they now get a WellFormed(ty::Infer(_)) obligation instead of silently ignoring the type.

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 2020
@eddyb eddyb mentioned this pull request Mar 19, 2020
Closed
@eddyb
Copy link
Member Author

eddyb commented Mar 19, 2020

@bors try @rust-timer queue (there may be new noop shallow_resolve calls introduced)

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Mar 19, 2020

⌛ Trying commit 1b15eb9a35ef930ba373fc945ee84dcabededdff with merge 05f512090ead730e044e52571dba2da5ff8f9dcf...

@eddyb
Copy link
Member Author

eddyb commented Mar 20, 2020

(I forgot to push some comment tweaks, hopefully it doesn't kill the try build)

@eddyb
Copy link
Member Author

eddyb commented Mar 20, 2020

@rust-timer build 05f512090ead730e044e52571dba2da5ff8f9dcf

@rust-timer
Copy link
Collaborator

Queued 05f512090ead730e044e52571dba2da5ff8f9dcf with parent f4c675c, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 05f512090ead730e044e52571dba2da5ff8f9dcf, comparison URL.

@eddyb
Copy link
Member Author

eddyb commented Mar 20, 2020

The worst regression, clap-rs-check clean has 143 less specializes calls? What's up with that?
But typeck_tables_of getting slower makes sense, and clap-rs-debug clean also has that.

I guess I'll try to come up with a synthetic stress test that hits the worst case of this PR (wf::obligations with ty::Infer that resolves to a different ty::Infer), and see what I can do to alleviate it.

But still, 7.5% regression of typeck_tables_of, from this change? That means a lot of clap-rs' type-checking time is spent on solving WF obligations.

@bors
Copy link
Contributor

bors commented Mar 21, 2020

☔ The latest upstream changes (presumably #70205) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2020
eddyb
eddyb commented Apr 3, 2020
View reviewed changes
src/librustc_trait_selection/traits/wf.rs
for upvar_ty in substs.as_closure().upvar_tys() {
// FIXME(eddyb) add the type to `walker` instead of recursing.
self.compute(upvar_ty);
Copy link
Member Author

@eddyb eddyb Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait I forgot about this, this is technically a bugfix (before, if compute returned false here, those inference variables would be forgotten about).

So it might make sense that it's slower. Let me, uh, make a PR that only changes this, and check that for perf regressions.

EDIT: Actually, there's the entirety of #70168, predicate_obligations being the other user of .compute w/o checking the return value. I might be able to pinpoint where the extra obligations are coming from.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is it.

typeck_tables_of on clap-rs gets slightly faster (instead of being a significant regression), if I ignore upvar types that are ty::Infer(ty::TyVar(_)) and don't resolve.

But that's a WF hole, right? Do we just accept the regression?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I lost track of this PR. I agree that this is a bug-fix.

@eddyb
Copy link
Member Author

eddyb commented Apr 3, 2020

I added some fast paths, let's see if they improve anything (but also see #70170 (comment)):

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Apr 3, 2020

⌛ Trying commit ef83aa34b11d92b10f53b514d04a5c2b475707d2 with merge bb0cbcbd0a0e408606210fa280cf86c90448764c...

@bors
Copy link
Contributor

bors commented Apr 3, 2020

☀️ Try build successful - checks-azure
Build commit: bb0cbcbd0a0e408606210fa280cf86c90448764c (bb0cbcbd0a0e408606210fa280cf86c90448764c)

@rust-timer
Copy link
Collaborator

Queued bb0cbcbd0a0e408606210fa280cf86c90448764c with parent 424c793, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit bb0cbcbd0a0e408606210fa280cf86c90448764c, comparison URL.

@eddyb
Copy link
Member Author

eddyb commented Apr 3, 2020 •
edited
Loading

Hehe, I think my second commit, wf: {Int,Float}Var can only infer to always-WF ints/floats., made the old macro-expanded version of inflate that perf.rust-lang.org tests, significantly faster to compile, it seems, so I guess it was spending a lot of time on WF-ing ty::IntVars?

I'm not surprised, there were a lot of macro-replicated integer literals in that code.

@lcnr lcnr mentioned this pull request Apr 9, 2020
Merged
@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 14, 2020
@nikomatsakis
Copy link
Contributor

I'm inclined to merge this PR @eddyb -- r=me on the code itself -- the clap-rs-debug perf regression seems small and I agree it's a bug fix, and there are some significant (albeit somewhat surprising?) improvements, too. I guess your explanation makes sense, though.

@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 30, 2020
@eddyb
Copy link
Member Author

eddyb commented May 1, 2020

Let's get some new numbers, post-rust-lang/rustc-perf#645:

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 1, 2020

⌛ Trying commit 05a872d with merge ad57f0a2528d29aca128300e3fa1b174a9eb497e...

@bors
Copy link
Contributor

bors commented May 1, 2020

☀️ Try build successful - checks-azure
Build commit: ad57f0a2528d29aca128300e3fa1b174a9eb497e (ad57f0a2528d29aca128300e3fa1b174a9eb497e)

@rust-timer
Copy link
Collaborator

Queued ad57f0a2528d29aca128300e3fa1b174a9eb497e with parent e94eaa6, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit ad57f0a2528d29aca128300e3fa1b174a9eb497e, comparison URL.

@eddyb
Copy link
Member Author

eddyb commented May 1, 2020

@bors r=nikomatsakis rollup=never

@bors
Copy link
Contributor

bors commented May 1, 2020

📌 Commit 05a872d has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 1, 2020
@bors
Copy link
Contributor

bors commented May 2, 2020

⌛ Testing commit 05a872d with merge 08dfbfb...

@bors
Copy link
Contributor

bors commented May 2, 2020

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 08dfbfb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 2, 2020
@bors bors merged commit 08dfbfb into rust-lang:master May 2, 2020
@eddyb eddyb deleted the wf-early-exit branch May 2, 2020 11:54
@aDotInTheVoid aDotInTheVoid mentioned this pull request Dec 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Potential WF unsoundness: predicate_obligations doesn't check compute's return type.
7 participants
@eddyb @rust-timer @bors @nikomatsakis @JohnCSimon @rust-highfive @joelpalmer